Accounts Refactor PR 1: Adding account -> [credential_id] mapping#2770
Accounts Refactor PR 1: Adding account -> [credential_id] mapping#2770citizen-stig wants to merge 7 commits intotheodore/multisig-upgradefrom
Conversation
| .accounts | ||
| .resolve_sender_address(&address, &credential_id, state) | ||
| .map_err(CoreModuleError::state_write)?; | ||
| .is_authorized_for(&address, &credential_id, state) |
There was a problem hiding this comment.
resolve_sender_address was being used for the side-effect of the credential id being registered for the address, it looks like that's no longer the case here. How would the credential id be registered now?
| // the payer address, so `is_authorized_for` succeeds via the canonical-address | ||
| // arm without needing any stored `accounts` or `account_owners` entry. This | ||
| // exercises the stateless happy path. | ||
| let embedded = payer; |
There was a problem hiding this comment.
This shouldn't be changed, the payer and embedded should explicitly be different
Explains a bit more about what the module is meant to achieve: https://github.com/Sovereign-Labs/sovereign-sdk/tree/dev/crates/module-system/module-implementations/extern/hyperlane-solana-register#message-structure
Namely:
The embedded public key becomes the CredentialId on the rollup, and it's associated with the rollup address derived from the payer's public key.
The result of this process is that the embedded wallet controls the users Solana address on the rollup. This is desirable in situations like Zetas where we want the embedded wallet to be opaque, with the user feeling like they're using their Solana wallet directly.
Description
PR 1 of the accounts refactor series. Introduces a new
account_ownersauthorization set alongside the existingaccountsmap and stops writing to the legacy map from every code path. The legacy map remains readable so pre-upgrade state keeps routing.What changed
account_owners: StateMap<(address, credential_id), bool>.InsertCredentialIdnow writesaccount_owners, notaccounts.account_owners.resolve_sender_addressbecomes read-only (no auto-create on miss).is_authorized(addr, cred)andis_authorized_for(addr, cred).accountsmap kept purely as a read-only legacy fallback. No new writers.hyperlane-solana-registermigrated fromresolve_sender_addresstois_authorized_for.booleaninstead of unit type()because of limitations of rockbound: Refactoring for more explicit sentinel values handling rockbound#50Trade-offs / behavioral deltas
InsertCredentialId(X)from senderAno longer makesXroute toA. It only authorizesXforAinaccount_owners. WhenXlater signs a V0 tx, the resolver returnsX.into()(stateless default), unless a pre-upgrade entry exists in legacyaccounts. Fund the canonical address (or rely on a pre-upgrade entry) if you needXto draw gas fromA.exit_if_credential_existsnow checks(sender, credential)inaccount_ownersinstead of global credential uniqueness inaccounts. Authorizing someone else's credential for your own address now succeeds — harmless, since they still need the private key to sign.get_account(credential_id)returnsAccountEmptyfor post-PR credentials. New writes don't land inaccounts. Aget_addresses_for_credentialquery for the new model ships in a follow-up; tooling owners should plan accordingly.is_authorized_forgives legacy mappings exclusive priority. If a credential has a legacyaccountsentry, only that address is authorized (canonical match andaccount_ownersare not consulted). Doc updated to reflect this; code relies on the invariant that no code path writes both maps for the same credential.Backward compatibility
accountsentries keep routing until explicitly migrated.AccountConfig,CallMessage, and tx schema unchanged.get_account(see above).Known gaps / deferred
DrainLegacyAccountsmigration andget_addresses_for_credentialquery land in a later PR.resolve_sender_addressis now pure-read but still takes&mut selfandStateAccessor, duplicatingresolve_sender_address_read_only. Two callers insov-capabilitiesneed migration before the mutable variant can be deleted — deferred.is_authorized_forlegacy-first precedence is not enforced in code; relies on "no writer touches both maps for the same credential". Fine today, brittle if a future migration ever violates it.I have updatedCHANGELOG.mdwith a new entry if my PR makes any breaking changes or fixes a bug. If my PR removes a feature or changes its behavior, I provide help for users on how to migrate to the new behavior.Cargo.tomlchanges before opening the PRs. (Are all new dependencies necessary? Is any module dependency leaked into the full-node (hint: it shouldn't)?)Linked Issues
sov-accountsrefactoring and extending #2772Testing
New tests added in
sov-accountsintegration tests (test_setup_multisig_and_act,test_resolve_address_with_multi_credential_ownership, etc.). Existingtest_multisig_signature_verificationinauth_eip712updated to seed the multisig's canonical address at genesis under the new model.Docs
sov-accounts/README.mdexpanded with the three-relation model (legacy mapping / stateless canonical / explicit authorization). Function docstrings updated.